Enable applying multiple Configurations that use the same Discovery Handler #432
Conversation
kate-goldenring
requested review from
bfjelds,
Britel,
jiria and
romoh
as code owners
kate-goldenring
force-pushed
the
multiple-configs-per-dh
branch
2 times, most recently
from
da7e2a5
to
650af7d
Compare
agent/src/util/discovery_operator.rs
Outdated
| @@ -674,25 +604,23 @@ pub mod start_discovery { | |||
| endpoint | |||
| ); | |||
| // Only use DiscoveryHandler if it doesn't have a client yet | |||
There was a problem hiding this comment.
seems like this comment should change
There was a problem hiding this comment.
good point thanks!
|
i don't remember exactly why we had a filter for endpoint=Active. is it possible that what we really wanted was (endpoint+configurationName)=Active? |
Sorry @bfjelds forgot to hit enter on the reply. I think it was to prevent multiple configurations from using the same discovery handler. I dont know why i thought that was something to prevent when we definitely want that to be enabled. I dont see a reason for tracking state as we can tell when we lose connection with the Discovery Handler |
Signed-off-by: Kate Goldenring <[email protected]>
Signed-off-by: Kate Goldenring <[email protected]>
Signed-off-by: Kate Goldenring <[email protected]>
kate-goldenring
force-pushed
the
multiple-configs-per-dh
branch
from
1c99478
to
23eafdf
Compare
| } | ||
| } else { | ||
| return Err(e); |
There was a problem hiding this comment.
I'm trying to see if this is a case we want to handle. Can status be empty? Maybe log an error at least?
There was a problem hiding this comment.
This error is hit when the type of error is not tonic::Status, signaling that it is not a connection error with the Discovery Handler rather one of the Discovery Operator's other functions that errored such as update_instance_connectivity_status. This is an error we'd want to bubble up while connection errors can be retried in case the discovery handler comes back online.
Do you think an error! log should be omitted before the bubble up?
There was a problem hiding this comment.
It might help logging from debuggability perspective. I was mainly curious how we want to handle that case and if we should treat it like a connection error.
Signed-off-by: Kate Goldenring <[email protected]>
Signed-off-by: Kate Goldenring [email protected]
What this PR does / why we need it:
closes #431
Multiple configurations should be able to be applied that use the same DiscoveryHandler in order to support deploying different types of brokers to devices and discovering different sets of devices. For example, you may want to deploy two Configurations that use the udev Discovery Handler as follows: one that discovers USB cameras and deploys a broker that grabs frames from the camera and another that discovers GPUs and deploys inferencing pods.
Special notes for your reviewer:
Removes the concept of
DiscoveryHandlerStatuswhich seemed unnecessary.Removed tests in
config_action.rsthat were actually failing due to improperly referencing theMockDiscoveryOperator. The DiscoveryOperator should be reworked into a trait structure so it can be more easily mocked.If applicable:
cargo fmt)cargo build)cargo clippy)cargo test)cargo doc)./version.sh)